Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-write Galaxy modeling code #157

Merged
merged 3 commits into from Aug 7, 2014

Conversation

adonath
Copy link
Member

@adonath adonath commented Jul 26, 2014

Revision of the galaxy source modelling code, including the following changes:

  • Implementation of the spatial and velocity distribution models as astropy.modeling.Fittable1DModel
  • Use of astropy.units.Quantity throughout the code
  • Added tests
  • Added minimal docs

from astropy.table import Table
from astropy.utils.data import get_pkg_data_filename

filename = get_pkg_data_filename('data/atnf_sample.txt')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails because the data/atnf_sample.txt file is not installed:
https://travis-ci.org/gammapy/gammapy/jobs/30936682#L974

You probably need to add this file via get_package_data in setup_package.py as described here ... you can find a few examples in other parts of Gammapy or Astropy.

t = self.age
else:
raise ValueError('Need time variable or age attribute.')
r = np.where(t > self.sedov_taylor_begin, self._radius_sedov_taylor(t).to('cm'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add line break after comma? (I find this a bit more readable.)

@cdeil
Copy link
Contributor

cdeil commented Jul 27, 2014

Thanks for cleaning all this up!

I did a review just based on reading the code.

Feel free to merge after making travis-ci pass and addressing the comment, or ping me again if you want me to have a second look or actually try this stuff out.

@cdeil
Copy link
Contributor

cdeil commented Jul 27, 2014

One thing that I don't like is that the radial distribution classes like FaucherKaspi2006 are simply listed in gammapy.astro.source and their name doesn't contain GalacticRadialDistribution or something that describes what they actually are.

The names get super-long if you write GalacticRadialDistributionFaucherKaspi2006, so I can see why you didn't do it, but still, it's not nice to have classes where the name doesn't describe at all what they are.
@adonath What do you think?

@cdeil
Copy link
Contributor

cdeil commented Aug 4, 2014

@adonath - I think Ellis wants to produce the main results for the SciNeGhe proceeding this week ... maybe it's possible to merge this on Thursday or Friday latest? (you can always make new PRs or smaller changes directly in master)

adonath added a commit that referenced this pull request Aug 7, 2014
@adonath adonath merged commit f30420f into gammapy:master Aug 7, 2014
@cdeil
Copy link
Contributor

cdeil commented Aug 7, 2014

Thanks!

@cdeil cdeil changed the title Galaxy modelling code revision Re-write Galaxy modelling code Apr 8, 2015
@cdeil cdeil added the feature label Apr 8, 2015
@cdeil cdeil added this to the 0.1 milestone Apr 8, 2015
@adonath adonath deleted the astropy_population_models branch November 20, 2018 08:37
@cdeil cdeil changed the title Re-write Galaxy modelling code Re-write Galaxy modeling code Sep 30, 2019
LauraOlivera pushed a commit to LauraOlivera/gammapy that referenced this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants